Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support cert by reference (#268) #269

Merged
merged 7 commits into from
Feb 13, 2021
Merged

Conversation

ikethecoder
Copy link
Contributor

No description provided.

@ikethecoder ikethecoder requested a review from a team as a code owner February 4, 2021 21:13
@CLAassistant
Copy link

CLAassistant commented Feb 4, 2021

CLA assistant check
All committers have signed the CLA.

file/builder.go Outdated
} else if err != nil {
b.err = err
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what is the case that you are trying to fix here?
I can read the diff but I can't understand why this is needed.

Copy link
Contributor Author

@ikethecoder ikethecoder Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Harry, sure.. the particular scenario we faced was that we want to setup mTLS between a Service and the Upstream endpoint, so a Client Cert/Key needs to be configured on the Gateway so that the Service can reference it. We also want to separate concerns so that the Client Cert/Key is updated in a different process than the Service/Route - for example having the Cert/Key updated/rotated by a security team, and the Service/Route updated by an application team. We use the --select-tag option as a way to segment configuration by different application teams. There is an example in issue #268 as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed the reference to that issue before (please include it in your PR message body next time).

So the use case you are trying to achieve makes a lot of sense.
Have you actually tested this patch against it?
The reason I ask is because currentState shouldn't have the Certificate either because it won't be part of the configuration based on select-tag.
I could be missing something here, it has been a while since I have dug in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested it; and there are a couple of unit tests included. You are correct that the currentState may not have the Certificate and that is why the extra check is done - it will throw an error if: (1) the Certificate is missing from the select-tag config or (2) the Certificate is missing from the currentState. It has to be in one or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry - I follow what you are saying now - you are saying that currentState is scoped as well by select-tag. Let me test that again then and see what is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I retested and you are correct - this looks like it is a bit more involved to get it supported :(.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Harry, have a look at the latest commit and see if you agree with the solution. I added code to pull in all certs into the state in a different table (allAvailableCertificates) and use that to validate the existence of the cert. If its not suitable, my only other option that I can think of would be to remove the certificate validation all together (basically would behave similarly to the ca_certificates attribute).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the prompt response here.

The current patch doesn't seem acceptable to me:

  • --select-tags is a hard filter. decK shouldn't go outside it's scope in most cases. Going out of scope for validation seems wrong.
  • You made a very good and helpful point around ca_certificate attribute. We should be treating both as same.

Amongst the two options, I think removing validation makes the most sense here. I do not like reducing validation but given the safety net that this will be caught by Kong's API later, this is okay.

Foreign-key references are hard to catch in decK and are performed on a best effort basis. I also think it makes sense to add the validation you have in your patch but it requires some more machinery in the code-base to ensure maintainability. Managing in-memory state in decK is very hairy and tricky, I don't want to further increase the tech debt in this area.

@ikethecoder Could you update the patch to remove the validation of Certificate from Service resource?

Note for future-selves:
Even though we are relaxing validation here, the same argument can't be used for relaxing validation for route-service or route/service/consumer-plugin foreign relations. Certificate and Service entity are loosely related when it comes to operations. There is a compelling use-case to relax this validation. I do not think separating routes from services or plugins from where are used makes much sense and should be avoided at all cost and is in the best interest of all users. Separating out these strongly related entities makes decK and Kong's UX much worse and can easily become a maintenance nightmare.

@ikethecoder
Copy link
Contributor Author

@hbagdi Great thanks Harry, I've updated the PR to just remove the validation check. Please review.

@@ -430,14 +430,6 @@ func (b *stateBuilder) services() {
utils.MustMergeTags(&s.Service, b.selectTags)
b.defaulter.MustSet(&s.Service)

if s.ClientCertificate != nil && !utils.Empty(s.ClientCertificate.ID) {
if _, ok := b.certIDs[*s.ClientCertificate.ID]; !ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems b.certIDs was used only in this one place and can now be completely removed.
Can you delete the following lines as well?

20:     certIDs      map[string]bool
48:     b.certIDs = map[string]bool{}
109:            b.certIDs[*c.ID] = true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, true! Have removed those lines.

@hbagdi hbagdi merged commit 2edc2d5 into Kong:main Feb 13, 2021
@hbagdi
Copy link
Member

hbagdi commented Feb 13, 2021

Thank you for your contribution. Please fill out the following form to claim your contributor swag:
https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt.

@ikethecoder ikethecoder deleted the feature/issue-268 branch February 16, 2021 17:29
rainest pushed a commit that referenced this pull request Apr 21, 2021
AntoineJac pushed a commit that referenced this pull request Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants